Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

assert: improve deep equal map #14501

Closed
wants to merge 2 commits into from
Closed

assert: improve deep equal map #14501

wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jul 26, 2017 •
edited
Loading

This improves the performance for maps with object keys and for deepEqual loose equal primitive keys that contain primitives as value. The first part decreases the performance of the common case a tiny bit (primitive keys) in favor of the much slower worst case. Even though the common case must do a typecheck more I think it is favorable as it is also better to read that way.
In addition I updated a few comments.

 assert/deepequal-map.js method="deepEqual_looseMatches" len=1000 n=1000               16.85 %        *** 1.740720e-44
 assert/deepequal-map.js method="deepEqual_mixed" len=1000 n=1000                       2.14 %        *** 4.011895e-04
 assert/deepequal-map.js method="deepEqual_objectOnly" len=1000 n=1000                  3.45 %        *** 4.751408e-12
 assert/deepequal-map.js method="deepEqual_primitiveOnly" len=1000 n=1000              -2.52 %         ** 2.808637e-03
 assert/deepequal-map.js method="deepStrictEqual_mixed" len=1000 n=1000                 3.00 %        *** 7.353710e-07
 assert/deepequal-map.js method="deepStrictEqual_objectOnly" len=1000 n=1000            3.28 %        *** 6.647043e-09
 assert/deepequal-map.js method="deepStrictEqual_primitiveOnly" len=1000 n=1000        -1.01 %            4.022507e-01
 assert/deepequal-map.js method="notDeepEqual_looseMatches" len=1000 n=1000            16.35 %        *** 8.956366e-27
 assert/deepequal-map.js method="notDeepEqual_mixed" len=1000 n=1000                   -1.30 %            5.267886e-01
 assert/deepequal-map.js method="notDeepEqual_objectOnly" len=1000 n=1000               4.62 %        *** 2.039886e-14
 assert/deepequal-map.js method="notDeepEqual_primitiveOnly" len=1000 n=1000           -1.48 %          * 3.809026e-02
 assert/deepequal-map.js method="notDeepStrictEqual_mixed" len=1000 n=1000              0.92 %            6.739208e-01
 assert/deepequal-map.js method="notDeepStrictEqual_objectOnly" len=1000 n=1000         5.44 %        *** 4.738282e-19
 assert/deepequal-map.js method="notDeepStrictEqual_primitiveOnly" len=1000 n=1000     -1.59 %            1.132915e-01
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jul 26, 2017
@andrasq andrasq added the performance Issues and PRs related to the performance of Node.js. label Jul 26, 2017
@jasnell
Copy link
Member

jasnell commented Jul 27, 2017

ping @mscdex

@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 8, 2017

PTAL

@jasnell jasnell requested review from mscdex and addaleax August 8, 2017 23:37
@mscdex
Copy link
Contributor

mscdex commented Aug 9, 2017 •
edited
Loading

Without having reviewed the changes, I'd say the performance numbers are ok. I would just say be sure to re-run those benchmarks if/when backporting this PR to v6 and/or v4 to make sure that there are no more serious regressions due to Crankshaft, etc.

@refack
Copy link
Contributor

refack commented Aug 9, 2017

Without having reviewed the changes, I'd say the performance numbers are ok.

Welcome back @mscdex 🤗

@refack refack self-assigned this Aug 13, 2017
refack pushed a commit that referenced this pull request Aug 13, 2017
@BridgeAR @refack
assert: optimize code path for deepEqual Maps
9222fe6 
PR-URL: #14501
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack
Copy link
Contributor

refack commented Aug 13, 2017 •
edited
Loading

Landed in 9222fe6
Extra sanity test of master: https://ci.nodejs.org/job/node-test-commit-linuxone/7939/ ✔️

@refack refack closed this Aug 13, 2017
addaleax pushed a commit that referenced this pull request Aug 13, 2017
@BridgeAR @addaleax
assert: optimize code path for deepEqual Maps
a6539ec 
PR-URL: #14501
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@addaleax addaleax mentioned this pull request Aug 13, 2017
Merged
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Sep 20, 2017
@MylesBorins
Copy link
Contributor

Not sure if we want to backport this to v6.x

@BridgeAR
Copy link
Member Author

The same as #14258
Map and Set was added in v8. as a semver-major. I add the do not land labels.

@BridgeAR BridgeAR added dont-land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Sep 20, 2017
@refack refack removed their assignment Oct 20, 2018
@BridgeAR BridgeAR deleted the improve-deep-equal-map branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

@mscdex mscdex Awaiting requested review from mscdex

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @jasnell @mscdex @refack @MylesBorins @andrasq @nodejs-github-bot